New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix false positives for parametric mixins in selector-class-pattern #5378
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request, and apologies for just getting around to reviewing it.
Can you please set to merge into the v14
branch.
Noticed that with the added if-block there is no necessity in checker on lines 59-61. Probably, should be removed? Furthermore, if to delete checkers on lines 21-44 in isStandardSyntaxRule.js all tests will be passed. Is it ok?
If the new conditional makes the previous ones redundant and the tests pass, then yes go ahead a remove them.
158e277
to
15cca41
Compare
Hi @immitsu, thank you for creating this PR! When I’m looking into For example, the following test case is close to this case added by this PR: stylelint/lib/utils/__tests__/isStandardSyntaxRule.test.js Lines 41 to 43 in 403248c
While -expect(isStandardSyntaxRule(lessNode('.mixin-name(@var);'))).toBeFalsy();
+expect(isStandardSyntaxRule(lessNode('.mixin-name(@var) {}'))).toBeFalsy(); So, I think we should fix the test and refactor the tested code, and then fix this false positive. What do you think? |
Hi @ybiquitous! I think you're right, have fixed/deleted inaccurate test cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@immitsu Thank you for addressing my reviews! I've left a few suggestions, so I would be glad if considering them. 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@immitsu Thank you for your work! LGTM 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
v14 changelog entry:
|
Fix #5258
Noticed that with the added if-block there is no necessity in checker on lines 59-61. Probably, should be removed?
Furthermore, if to delete checkers on lines 21-44 in
isStandardSyntaxRule.js
all tests will be passed. Is it ok?